Skip to content

Conversation

seisman
Copy link
Member

@seisman seisman commented Jul 14, 2025

This PR implements the Box class that can be used in modules like coast/image/legend/colorbar/....

The full GMT CLI syntax is (https://docs.generic-mapping-tools.org/6.6/colorbar.html#f):

-F[+cclearances][+gfill][+i[[gap/]pen]][+p[pen]][+r[radius]][+s[[dx/dy/][shade]]]

The upstream long-form names are available at:

https://github.com/GenericMappingTools/gmt/blob/7026382558e11f89e2238bb345e1d0af325c12aa/src/longopt/psscale_inc.h#L35

  • +c: clearance
  • +f: fill
  • +i: inner
  • +p: pen
  • +r: radius
  • +s: shade

The Box class follows the upstream long-form names, but use inner_gap/inner_pen for +i and shade_offset/shade_fill for +s.

Preview:

@seisman seisman force-pushed the AliasSystem/params/base branch 4 times, most recently from dd01578 to d9ccd1f Compare July 14, 2025 07:21
@seisman seisman force-pushed the AliasSystem/params/base branch from 311c83f to 926a690 Compare July 29, 2025 04:39
@seisman seisman added the feature Brand new feature label Aug 3, 2025
@seisman seisman added this to the 0.17.0 milestone Aug 3, 2025
@seisman seisman added the needs review This PR has higher priority and needs review. label Aug 3, 2025
@seisman
Copy link
Member Author

seisman commented Sep 5, 2025

@GenericMappingTools/pygmt-maintainers Please give this PR a high priority, since many PRs are waiting for it.

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just two suggestions, one on ordering the docs, another one on making class BaseParam and AbstractBaseClass.

"""


class BaseParam:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should make this an AbstractBaseClass (was debating between ABC or a Protocol).

Suggested change
class BaseParam:
from abc import ABC, abstractmethod, abstractproperty
class BaseParam(ABC):

Copy link
Member Author

@seisman seisman Sep 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've applied the suggestions but also made some changes in c6bd16f. The main changes are:

  • abstractproperty is deprecated https://docs.python.org/3/library/abc.html#abc.abstractproperty. Now use @property and @abstractmethod.
  • @abstractmethod means subclasses must implement the method/property, so the NotImplementedError is not needed and can be removed.
  • The _validate method is meant to be optional, so we can't use @abstractmethod. Then ruff reports the B027 error, and it's ignored by adding the noqa flag.

seisman and others added 4 commits September 7, 2025 16:47
- abstractproperty is deprecated
- _validate is optional, so we can't use @AbstractMethod
- improve docstrings
@seisman seisman added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Sep 7, 2025
@seisman seisman changed the base branch from main to dvc/debug September 7, 2025 13:30
@seisman seisman changed the base branch from dvc/debug to main September 7, 2025 13:30
@seisman seisman changed the base branch from main to dvc/debug September 7, 2025 13:43
Base automatically changed from dvc/debug to main September 7, 2025 22:09
@seisman seisman merged commit 2200457 into main Sep 8, 2025
22 of 24 checks passed
@seisman seisman deleted the AliasSystem/params/base branch September 8, 2025 03:39
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Sep 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Brand new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants